Skip to content

Arm64: Fix for 96441 #123022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
dhartglassMSFT wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Arm64: Fix for 96441 #123022

dhartglassMSFT wants to merge 7 commits into from
+111 −38

Conversation

@dhartglassMSFT
Copy link

@dhartglassMSFT dhartglassMSFT commented Jan 9, 2026 •
edited
Loading

The increment of lzcnt (*) wasn't getting transformed to a conditional increment, because an earlier phase folds the "add local, 1" into a constant based on assertion information.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int RuneLength(in byte value)
{
    var lzcnt = (uint)BitOperations.LeadingZeroCount(~((uint)value << 24));
    if (lzcnt is 0) lzcnt++; //*

    return (int)lzcnt;
}

Fix to allow TryLowerCnsIntCselToCinc to look cases similar to if (myvar==0)myvar=1;

fixes #96441

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 9, 2026
@dhartglassMSFT
Copy link
Author

dhartglassMSFT commented Jan 9, 2026

@dotnet-policy-service agree company="Microsoft"

@build-analysis build-analysis bot mentioned this pull request Jan 9, 2026
Open
@build-analysis build-analysis bot mentioned this pull request Jan 22, 2026
Open
3 tasks
@dhartglassMSFT dhartglassMSFT marked this pull request as ready for review January 26, 2026 23:58
@dhartglassMSFT dhartglassMSFT changed the title [DRAFT] Generate more CINC Arm64: Fix for 96441 Jan 27, 2026
Copilot AI reviewed Jan 27, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Expands ARM64 lowering to produce more conditional-increment (cinc/csinc) patterns, especially for assignments derived from constants compared against a local.

Changes:

  • Extend TryLowerCnsIntCselToCinc to also recognize SELECTCC patterns where the “+1” value is constant relative to a CMP local/constant comparison, and rewrite to GT_SELECT_INCCC.
  • Broaden LowerSelect’s trigger to attempt this lowering when either select operand is an integer constant (not only when both are).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/jit/lowerarmarch.cpp Adds a new lowering path to convert certain SELECTCC + CMP patterns into GT_SELECT_INCCC by substituting a local read for the constVal+1 constant.
src/coreclr/jit/lower.cpp Calls the CINC lowering helper when either select operand is an integer constant, enabling the new pattern matching.

src/coreclr/jit/lowerarmarch.cpp
return;
}

// One of the CMP operands is a constant int
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the non-constant/constant path, this code calls select->AsOpCC() and later mutates the node to GT_SELECT_INCCC, but TryLowerCnsIntCselToCinc is declared to accept both GT_SELECT and GT_SELECTCC. Please add an explicit guard/assert that select is GT_SELECTCC before calling AsOpCC() (or restructure so this path can’t be reached for GT_SELECT) to avoid UB if the precondition ever changes.

Suggested change
// One of the CMP operands is a constant int
// One of the CMP operands is a constant int
if (!select->OperIs(GT_SELECTCC))
{
// This transformation currently applies only to GT_SELECTCC nodes.
return;
}

Copilot uses AI. Check for mistakes.
This was referenced Jan 27, 2026
Open
Open
Open
Open
Open
Open
Open
jakobbotsch
jakobbotsch reviewed Jan 27, 2026
View reviewed changes
jakobbotsch
jakobbotsch approved these changes Jan 27, 2026
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I will kick off Fuzzlyn which is good at testing stuff like this (it may have failures in it unrelated to this PR)

@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 27, 2026

/azp run Fuzzlyn

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 27, 2026

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jan 28, 2026
Open
3 tasks
@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 28, 2026

I went through the Fuzzlyn failures (https://dev.azure.com/dnceng-public/public/_build/results?buildId=1268916&view=ms.vss-build-web.run-extensions-tab) and I don't see anything that obviously looks to be caused by this PR. I think you can bypass the infra issues and merge this.

@dhartglassMSFT dhartglassMSFT changed the title Arm64: Fix for 96441 Arm64: fixes #96441 Jan 28, 2026
@dhartglassMSFT dhartglassMSFT changed the title Arm64: fixes #96441 Arm64: Fix for 96441 Jan 28, 2026
@dhartglassMSFT dhartglassMSFT enabled auto-merge (squash) January 28, 2026 19:40
This was referenced Jan 29, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@dhartglassMSFT dhartglassMSFT

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ARM64] CINC or CSINC is not emitted for conditional increment

2 participants

@dhartglassMSFT @jakobbotsch